packages/rest-api/src/controllers/swapController.ts (1)

46-46: Document response schema changes

The addition of callData to the response payload is a breaking change that needs proper documentation and potentially a version bump.

Please ensure you:

1. Update OpenAPI/Swagger documentation to reflect the new response format
2. Consider versioning the API endpoint (e.g., /v2/swap) to maintain backward compatibility
3. Add examples in the API documentation showing both cases (with and without address)

packages/rest-api/src/controllers/bridgeController.ts (2)

49-82: Consider performance optimization for response processing

The current implementation processes quotes sequentially within Promise.all. For large response sets, this could impact performance.

Consider parallel processing of independent operations:

const processQuote = async (quote) => {
  const [originQueryTokenOutInfo, callData] = await Promise.all([
    Promise.resolve(tokenAddressToToken(fromChain.toString(), quote.originQuery.tokenOut)),
    destAddress ? Synapse.bridge(...) : Promise.resolve(null)
  ]);
  return {
    ...quote,
    maxAmountOutStr: formatBNToString(quote.maxAmountOut, toTokenInfo.decimals),
    bridgeFeeFormatted: formatBNToString(quote.feeAmount, originQueryTokenOutInfo.decimals),
    callData,
  };
};

---

49-82: Consider implementing pagination for large response sets

The response processing loads all quotes into memory simultaneously. For large datasets, this could cause memory issues.

Consider implementing pagination:

1. Add limit and offset query parameters
2. Process only the requested subset of quotes
3. Return metadata about total available quotes

packages/rest-api/src/tests/swapRoute.test.ts (4)

137-154: Consider parameterizing the test wallet address.

The test provides good coverage of the callData structure, but using a hardcoded wallet address might make the test brittle. Consider moving the address to a test constants file or using a deterministic test wallet.

+// At the top of the file with other imports
+import { TEST_WALLET_ADDRESS } from '../constants/test'

-      address: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e',
+      address: TEST_WALLET_ADDRESS,

---

156-169: Add descriptive comment explaining the null callData behavior.

The test correctly verifies the default behavior, but it would be helpful to document why callData is null when no address is provided.

+  // When no address is provided, callData should be null as transaction details
+  // cannot be constructed without a destination address
   it('should return swap quote without callData when address is not provided', async () => {

---

171-182: Enhance invalid address test coverage.

Consider adding more test cases for different types of invalid addresses:

• Zero address (0x0000...)
• Almost valid address (wrong checksum)
• Address with invalid length

Example addition:

it.each([
  ['0x0000000000000000000000000000000000000000', 'Zero address not allowed'],
  ['0x742d35Cc6634C0532925a3b844Bc454e4438f44', 'Invalid address length'],
  ['0x742d35cc6634c0532925a3b844bc454e4438f44e', 'Invalid checksum']
])('should return 400 for invalid address: %s', async (address, scenario) => {
  const response = await request(app).get('/swap').query({
    chain: '1',
    fromToken: USDC.addresses[1],
    toToken: DAI.addresses[1],
    amount: '1000',
    address,
  })

  expect(response.status).toBe(400)
  expect(response.body.error).toHaveProperty('message', 'Invalid address')
})

---

136-182: Consider grouping related test cases.

The new address-related tests would be better organized using a describe block to group them together.

+  describe('address parameter handling', () => {
     it('should return swap quote with callData when address is provided', async () => {
       // ... existing test
     })

     it('should return swap quote without callData when address is not provided', async () => {
       // ... existing test
     })

     it('should return 400 for invalid address', async () => {
       // ... existing test
     })
+  })

packages/rest-api/src/routes/swapRoute.ts (1)

196-199: Consider normalizing the address parameter.

The validation for the optional address parameter is well implemented. However, for consistency with other address parameters in the route (fromToken and toToken), consider adding the address parameter to the checksumAddresses middleware.

 router.get(
   '/',
   normalizeNativeTokenAddress(['fromToken', 'toToken']),
-  checksumAddresses(['fromToken', 'toToken']),
+  checksumAddresses(['fromToken', 'toToken', 'address']),
   [

packages/rest-api/src/tests/bridgeRoute.test.ts (3)

177-194: Consider enhancing test coverage with additional assertions.

While the test effectively verifies the presence of callData and its structure, consider adding:

1. Type checks for callData properties
2. Assertions for maxAmountOutStr and bridgeFeeFormatted to maintain consistency with other test cases

 expect(response.body[0].callData).toHaveProperty('to')
 expect(response.body[0].callData).toHaveProperty('data')
 expect(response.body[0].callData).toHaveProperty('value')
+expect(typeof response.body[0].callData.to).toBe('string')
+expect(typeof response.body[0].callData.data).toBe('string')
+expect(typeof response.body[0].callData.value).toBe('string')
+expect(response.body[0]).toHaveProperty('maxAmountOutStr')
+expect(response.body[0]).toHaveProperty('bridgeFeeFormatted')

---

196-209: Maintain consistency with other test cases.

Consider adding assertions for maxAmountOutStr and bridgeFeeFormatted to maintain consistency with other test cases in the file.

 expect(response.body.length).toBeGreaterThan(0)
 expect(response.body[0].callData).toBeNull()
+expect(response.body[0]).toHaveProperty('maxAmountOutStr')
+expect(response.body[0]).toHaveProperty('bridgeFeeFormatted')

---

211-223: Ensure error response structure consistency.

To maintain consistency with other error cases in the file (e.g., invalid originUserAddress), consider adding an assertion for the error object structure.

 expect(response.status).toBe(400)
+expect(response.body).toHaveProperty('error')
 expect(response.body.error).toHaveProperty('message', 'Invalid destAddress')

packages/rest-api/src/routes/bridgeRoute.ts (3)

59-64: Enhance OpenAPI documentation for destAddress parameter.

While the documentation is technically correct, consider adding more details:

• Specify that it expects an EVM-compatible address
• Add an example value
• Clarify if this is different from originUserAddress and when to use each

 *       - in: query
 *         name: destAddress
 *         required: false
 *         schema:
 *           type: string
-*         description: The destination address for the bridge transaction
+*         description: The EVM-compatible destination address where bridged tokens will be sent. If not provided, tokens will be sent to the origin user's address.
+*         example: "0x742d35Cc6634C0532925a3b844Bc454e4438f44e"

---

110-119: Add descriptions and examples for callData response field.

The new callData object in the response schema needs more context:

• Add descriptions for each field
• Include example values
• Explain when this object will be null

 *                   callData:
 *                     type: object
 *                     nullable: true
 *                     properties:
 *                       to:
 *                         type: string
+*                         description: The target contract address for the bridge transaction
+*                         example: "0x742d35Cc6634C0532925a3b844Bc454e4438f44e"
 *                       data:
 *                         type: string
+*                         description: The encoded function call data for the bridge transaction
+*                         example: "0x095ea7b3000000000000000000000000..."
 *                       value:
 *                         type: string
+*                         description: The amount of native tokens to send with the transaction (in wei)
+*                         example: "0x0"
+*                     description: Transaction details returned when destAddress is provided, null otherwise

---

258-261: Consider additional validation rules for destAddress.

While the basic address validation is good, consider adding these safety checks:

• Prevent setting destAddress same as originUserAddress to avoid confusion
• Add chain-specific address validation if different chains use different address formats

     check('destAddress')
       .optional()
       .custom((value) => isAddress(value))
-      .withMessage('Invalid destAddress'),
+      .withMessage('Invalid destAddress')
+      .custom((value, { req }) => {
+        if (value && req.query.originUserAddress && 
+            value.toLowerCase() === req.query.originUserAddress.toLowerCase()) {
+          throw new Error('destAddress cannot be the same as originUserAddress');
+        }
+        return true;
+      }),

packages/rest-api/swagger.json (1)

200-208: Consider enhancing parameter documentation.

While the parameter descriptions are clear, consider adding:

1. Example values in the parameter definitions
2. More detailed descriptions explaining the expected format (e.g., if it should be a checksummed Ethereum address)
3. Common use cases or scenarios

 {
   "in": "query",
   "name": "destAddress",
   "required": true,
   "schema": {
     "type": "string"
   },
-  "description": "The destination address of the user on the destination chain"
+  "description": "The destination address of the user on the destination chain. Must be a valid, checksummed Ethereum address (e.g., 0x1234...abcd) where the bridged tokens will be received.",
+  "example": "0x742d35Cc6634C0532925a3b844Bc454e4438f44e"
 }

Also applies to: 1218-1226

Configuration used: .coderabbit.yaml
Review profile: CHILL

Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-09-17T23:09:10.071Z
Learning: The `/swap` route does not accept an `address` parameter.

Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-10-08T15:41:48.274Z
Learning: The `/swap` route does not accept an `address` parameter.

Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-09-17T23:09:10.071Z
Learning: The `/swap` route does not accept an `address` parameter.

Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-10-08T15:41:48.274Z
Learning: The `/swap` route does not accept an `address` parameter.

Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-10-08T15:41:49.333Z
Learning: The `/swap` route does not accept an `address` parameter.

Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-10-08T15:41:49.333Z
Learning: The `/swap` route does not accept an `address` parameter.

#!/bin/bash
# Search for destAddress validation in route files
rg -l "destAddress.*validate" --type ts

check('destAddress')
  .optional()
  .isEthereumAddress()
  .withMessage('Invalid destination address format')

#!/bin/bash
# Description: Verify API documentation and error handling consistency

# Test 1: Check if destAddress is documented in swagger
echo "Checking swagger documentation for destAddress..."
rg -A 5 "destAddress" "**/swagger.json"

# Test 2: Check error handling consistency across routes
echo "Checking error handling consistency..."
rg -A 2 "response.status).toBe\(400\)" "**/*.test.ts"

#!/bin/bash
# Let's try a different approach to verify API documentation and error handling

# Check for OpenAPI/Swagger documentation files with different extensions
echo "Checking for API documentation files..."
fd -e yaml -e yml -e json . | grep -i 'swagger\|openapi'

# Check the route handler implementation
echo "Checking bridge route implementation..."
rg -l "destAddress" "packages/rest-api/src"

# Check error handling patterns
echo "Checking error handling patterns..."
rg "status\(400\)" "packages/rest-api/src"

# Look for validation logic
echo "Checking validation logic..."
rg "destAddress.*valid" "packages/rest-api/src"

#!/bin/bash
# Let's check the API documentation and validation implementation

# Check the bridge route API documentation
echo "Checking bridge route API documentation..."
cat "packages/rest-api/swagger.json"

# Check the validation implementation in bridge route
echo "Checking bridge route validation..."
cat "packages/rest-api/src/routes/bridgeRoute.ts"

# Check the controller implementation
echo "Checking bridge controller implementation..."
cat "packages/rest-api/src/controllers/bridgeController.ts"

#!/bin/bash
# Check for related changes in other files
echo "Checking for related bridge endpoint changes..."
rg -l "bridge.*endpoint" --type ts

echo "Checking for other files importing bridgeRoute..."
rg -l "from ['\"].*bridgeRoute" --type ts

#!/bin/bash
# Check PR title and description for DNM context
gh pr view --json title,body

# Check commit messages for additional context
git log --pretty=format:"%s" -n 5

# Check for recent changes in bridge-related files
git diff HEAD~1 packages/rest-api/src/routes/bridgeRoute.ts || true

#!/bin/bash
# Check PR title and description for DNM context
gh pr view --json title,body

# Check for any pending changes or TODOs in the codebase
rg -i "todo|fixme|xxx|hack" --type ts packages/rest-api/

# Check for any related test files and their status
find packages/rest-api/src/tests -name "*bridge*.test.ts" -type f -exec cat {} \;